Skip to content

Bitreq client builder (rustls + native-tls)#516

Open
APonce911 wants to merge 4 commits intorust-bitcoin:masterfrom
APonce911:bitreq-client-builder-native-tls
Open

Bitreq client builder (rustls + native-tls)#516
APonce911 wants to merge 4 commits intorust-bitcoin:masterfrom
APonce911:bitreq-client-builder-native-tls

Conversation

@APonce911
Copy link
Contributor

@APonce911 APonce911 commented Feb 23, 2026

Summary

This PR addresses issue #473 by creating a Client Builder able to receive custom root certificates at runtime.
The ClientBuilder requires async-https-rustls or async-https-native-tls features.

Changes

  • Add Client Builder pattern.
  • Creates Certificates wrapper module.
  • Append certificate using with_root_certificate builder method.
  • Disable default certificates using disable_default_certificates builder method.
  • Load certificates once per client instead of per connection.

This PR substitutes the 502

@APonce911 APonce911 marked this pull request as ready for review February 23, 2026 23:06
@tcharding
Copy link
Member

tcharding commented Feb 24, 2026

Hi @APonce911, little comment on dev process. Its better if you rebase on master instead of merging if you want review. With 65 patches reviewers are not going to know where to look. Many of us review by pulling down the PR and reading each patch in our terminals.

Different folk treat things differently but to me if a PR is 'open' it is a request to merge so it should be in a mergable state. We use the process where each commit should be a single logical change. There shouldn't be WIP commits and 'fix something I did three commits ago' commits. The whole set of commits (aka the 'patch set') should be clean and reviewable as stand alone changes. Each described in the commit log.

@APonce911
Copy link
Contributor Author

@tcharding thanks for the feedback. That's something I'll keep in mind! As I've told you, I'm not used to open source so every bit of insight of how you do things is helpful. The process is a bit different than what I'm used to but I'm more than happy to adapt.

About those patches and then small subsequent fixes, I've had a hard time trying to replicate the CI suite locally, it didn't catch some corner cases. I could have reset some commits, but didn't think about it by that time.

@tcharding
Copy link
Member

In a perfect world every commit builds and passes CI cleanly. We do not however check each commit in CI. But locally during dev you should really, in my opinion, be trying to make sure that at least cargo test --all-features and cargo test --no-default-features run cleanly on each commit [0]. That is the commands I default to and I tend to rely on CI to catch my other mistakes. Your mileage may vary. I don't even run just sane in bitcoin because I'm too impatient, but I more-or-less have a gut feel for what sorts of changes will break what test commands so I amend them if I need to (adding features ect.) Its not a perfect system by any means.

[0] I have shell alias' for these

@tcharding
Copy link
Member

If you want to get past the ci-doesn't-run-for-first-time-contributors just throw up a quick docs fix PR and I'll merge it.

@APonce911
Copy link
Contributor Author

@tcharding will definitely do that! However I will be off for a few days and won't be able to proceed with the rebase adjustments and doc pr until next week. Thanks for you time again!

@APonce911 APonce911 force-pushed the bitreq-client-builder-native-tls branch from 1e36aae to 27eac43 Compare March 3, 2026 19:40
Copy link
Member

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay this is starting to look pretty good. Aside from the above and below, we now need to clean up the git history...You can try rebasing, but it might at this point be simpler to just git reset upstream/master and then manually re-commit what you have.

@APonce911 APonce911 force-pushed the bitreq-client-builder-native-tls branch from ee83cc2 to 1ad18eb Compare March 6, 2026 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants